Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON Serialization Improvements #936

Merged
merged 20 commits into from
Jan 12, 2020
Merged

JSON Serialization Improvements #936

merged 20 commits into from
Jan 12, 2020

Conversation

Gerrit0
Copy link
Collaborator

@Gerrit0 Gerrit0 commented Jan 2, 2019

This is a rework of the JSON serialization, completing the work done in #597 and removing the deprecated .toObject methods.

The JSON schema retains the old structure, which is now described by the JSONOutput namespace.

closes #930
closes #728
closes #605

@jonchardy jonchardy mentioned this pull request Jan 14, 2019
Copy link
Collaborator

@aciccarello aciccarello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning out the duplicated code. I would like to see if we can simplify this.

*
* For documentation on the JSON output properties, view the corresponding model.
*/
export namespace JSONOutput {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation to have this as a namespace? Should we have a top level model exporting these (i.e. typedoc/serialization)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe my idea was to avoid polluting the global scope with ~40 new types.

The new top level export makes sense, but I think the way to do it should probably be to not publish TypeDoc within the dist/lib folder, this would be a massive breaking change, since basically all plugins import the Component decorator from typedoc/dist/lib/...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved these out of the namespace, there was no need as avoiding the pollution could be done by re-exporting under a specific name later. I still think the typedoc/serialization module is a good idea, but again, breaking changes that don't need to happen yet & should be thought through carefully.

*/
export type ModelToObject<T> = T extends Array<infer U> ? _ModelToObject<U>[] : _ModelToObject<T>;

// Order matters here. Some types are subtypes of other types.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an important note. Thanks for calling it out.

I was hoping that we could have some type of keyof mapping that might be simpler but it doesn't sound like that's possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With some changes to the JSON output, it would be possible, but for the initial typing I decided to just maintain the existing output types.

* Return a raw object representation of this reflection.
* @deprecated Use serializers instead
*/
toObject(): any {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to stub this with a call to the real serializer? This would help those who are looking for the serializer on the ProjectReflection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible, yes, but I'm not sure it is a good idea. To make it work we have 2 options.

  1. Construct a new serializer whenever .toObject is called
  2. Add an application: Application property to every reflection so that it can access the real serializer.

There are problems with both.

  1. The new serializer will be missing any extra serializers added by plugins
  2. Requires changing every reflection, and reflections really shouldn't know about serializers anyways (at least, that's the theory with having separate serialization...)

Since .toObject has been deprecated for nearly 2 years, I feel pretty ok about just removing the method.

title: M.ReflectionGroup['title'];
kind: M.ReflectionGroup['kind'];
children?: M.ReflectionGroup['children'][number]['id'][];
categories?: ModelToObject<M.ReflectionGroup['categories']>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move these interfaces to the individual serializers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it absolutely makes sense 👍 Will do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this, I'm not sure it does make sense. It's still an option, but having all the types in one place lets us avoid importing the S helper I introduced to resolve another of your comments everywhere (and thus keeps that particular bit of ugly out of the rest of of the code)

On the other hand, it would be nice to have the types closer to the code that produces values of that type...

type?: ModelToObject<M.SignatureReflection['implementationOf']>;
overwrites?: ModelToObject<M.SignatureReflection['implementationOf']>;
inheritedFrom?: ModelToObject<M.SignatureReflection['implementationOf']>;
implementationOf?: ModelToObject<M.SignatureReflection['implementationOf']>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of these look like they could be simplified using a keyof mapping.

Something like

type SignatureReflection = SimpleSerialization<Reflection>

This would help avoid issues when refactoring property names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did I miss the names here... I was avoiding keyof since for some reflections, not all properties are exported (notably, parent is absent as JSON can't include circular data structures)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, while it is certainly possible to use Exclude on the keyof result, this results in types which are displayed in a much less user friendly manner. The redundancy hurts, but I think it is probably worth it for types that can be easily explored.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have common patterns (like parent) there should be a way to exclude that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a shot, looking into preventing specific properties from being serialized and I don't think it is the right way to go about this.

  1. The list changes for (almost) every serialized reflection.
  2. As soon as a new property is added to a reflection, the serializer types have to be updated, even though nothing changed there.

Instead I introduced a helper S local to this file that handles basic cases. It behaves similarly to Pick, but passes properties through ModelToObject if necessary. Thoughts?

It did nothing more than Assert.deepStrictEqual, since if the objects weren't exactly the same deepEqual would throw.
- Move JSON types out of the namespace
- Introduce a helper to semi-automatically conver types to their JSON equivalent
- Address all the build errors caused by removing the namespace
@Gerrit0 Gerrit0 added this to the 0.16.0 milestone Nov 3, 2019
@Gerrit0 Gerrit0 merged commit 5a2fac6 into master Jan 12, 2020
@Gerrit0 Gerrit0 deleted the fix/serializer-plugin branch January 14, 2020 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants